Add Emscripten static library build CppInterOp #641
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #641 +/- ##
=======================================
Coverage 78.77% 78.77%
=======================================
Files 9 9
Lines 3849 3849
=======================================
Hits 3032 3032
Misses 817 817 🚀 New features to boost your workflow:
|
|
clang-tidy review says "All clean, LGTM! 👍" |
5 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
mcbarton
force-pushed
the
static-library
branch
2 times, most recently
from
e82078f to
5fdd505
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
mcbarton
force-pushed
the
static-library
branch
3 times, most recently
from
4d2a727 to
93f4518
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hi, could you try out all xeus-cpp-lite examples (from our link) based on top of the static library for cppinterop. |
Hi @anutosh491 Once this PR is in. I plan to put a PR in xeus-cpp to allow CppInterOp to have a deployment based on both a shared and static build, so we can compare both side by side (this will take one subsequent PR in CppInterOp to make this happen). Can you review and approve this PR if happy, so I can take it in once the ci passes? |
mcbarton
changed the title
Well we made quite some changes to jupyterlite (as xeus-cpp-lite was the first kernel using a side module) like this jupyterlite/xeus#146 for eg. So yeah I am a bit skeptical if both can live simultaneously. So I think it would be better if we could try it out before (also I am not sure we want to give users an option anyways, I would just stick to the better one) |
Each site would be built separately. There is no chance of a clash. This will be clear from the follow up to this PR in CppInterOp and the xeus-cpp one. This PR also doesn't in any way effect the xeus-cpp deployment. Please review whether you are happy with the Emscripten CppInterOp static library build. |
|
Not sure you understood, what I tried to convey. What I wanted to convey is that
Anyways on leave till the next 2 days after which I can look at this |
vgvassilev
reviewed
Jun 24, 2025
Traditionally people go for the static library builds , because Emscripten didn't support shared library builds in the past. This is no longer the case. We would need xeus-cpp to accept CppInterOp being a shared library in the future, even if a static library worked, if we want to include cppyy in the future deployment, since can only work with a shared library of CppInterOp. Here is the things I noticed most in how an Emscripten static library build compares to a shared library one. There are probably many more differences, but these are the things that stuck out to me
|
Either the other 80Mb must be hidden into something else that we don't measure, or we are doing something wrong in the shared object case... |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
Would be great if you could try this out, and test out all 3 examples notebooks with all relevant browsers (and devices you would have) I can pitch in soon trying out some devices on my end, once you atleast tell me if the static build gives us a working xeus-cpp-lite. |
@vgvassilev @anutosh491 I can test the Emscripten build of xeus-cpp, with this static library build of CppInterOp, but that is a separate issue to this PR, and would like it reviewed independent of this objective. This objective of this PR is purely about adding the ability to build CppInterOp as a Emscripten static library, and would like to keep that way. |
vgvassilev
approved these changes
Jun 25, 2025
This is might be ready but not sure if its a separate problem overall. The only library that essentially puts an emscripten build of cppinterop to use is xeus-cpp. If you have this built locally, it would be natural to test anything and everything that we can do with the shared build ... even with the static build. Rather if you get the notebooks working with better benchmarkings and everything works better overall, it gives it much more reason to switch a static build isn't it ? Okay this might be working on CI, but finally we need to put it to test correct ? P.S: There are other reasons I say so. Once I heard that xeus-cpp-lite doesn't work in Safari/chrome on an IPhone, I pinged maintainers of all other jupyterlite kernels. And none of the python/r or our cpp kernel work on an IPhone ... so it might not be a xeus-cpp problem at all (the whole running stuff on the phone scenario) and might be a Jupyterlite thing itself. So if you can use a static link and build xeus-cpp-lite and have it check it through different browsers/devices it would be good. |
|
I would say it would be a good idea to check if this can even be put to use ! |
|
We have tests which are mostly okay, right? I agree with your arguments but I think they apply for the case where we switch the deployment from shared to static builds. What do we gain from delaying this further? |
anutosh491
reviewed
Jun 25, 2025
| TEST(InterpreterTest, Process) { | ||
| #ifdef EMSCRIPTEN_STATIC_LIBRARY | ||
| GTEST_SKIP() << "Test fails for Emscipten static library build"; | ||
| #endif |
There was a problem hiding this comment.
Like literally Process which is at heart of xeus-cpp's code execution fails ?
Not sure if it makes sense to skip this out of all the possible tests :)
|
Anyways, feel free to take it in if everything's ready. Let's discuss more when we want to move to a static build. |
|
Just for the record, I don't think this build can be put to use for xeus-cpp just yet. Tried it out and we go back to the same issue, why we switched to a shared build in the first place compiler-research/xeus-cpp#159 This has no symbol out of llvm/clang cause the static archive only contains stuff out of Cppinterop We need to do something like this to fetch all the symbols #493 (comment) |
|
That explains the small size we see. The symbols are optimized away. |
Yes. While building this in verbose, cmake also produces a directory to keep track of stripped symbols. Has all the above symbols and more. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of changes, motivation and context for this PR.
Fixes # (issue)
CppInterOps Emscripten library can now be compiled to a static library, and pass almost all the same tests as the shared library build. This PR adds what is necessary to build the Emscripten static library, and run the tests on this build.
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist